Skip to content

Rewrite the configuration package (very basic for now) - #98

Merged
jcxldn merged 22 commits into
feat/jcx/major/modules-deffrom
feat/modules-pr/config
Mar 17, 2021
Merged

Rewrite the configuration package (very basic for now)#98
jcxldn merged 22 commits into
feat/jcx/major/modules-deffrom
feat/modules-pr/config

Conversation

@jcxldn

@jcxldn jcxldn commented Mar 5, 2021

Copy link
Copy Markdown
Member

No description provided.

@jcxldn jcxldn added dependencies Pull requests that update a dependency file breaking p0 bug Very high priority bug (server crashing etc) labels Mar 5, 2021
@jcxldn
jcxldn requested a review from kokumaji March 5, 2021 12:44
@jcxldn jcxldn self-assigned this Mar 5, 2021

@kokumaji kokumaji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add something like YamlProvider#reload()? Other than that, lgtm 👍
Waiting for opinions from others

@kaylendog
kaylendog self-requested a review March 5, 2021 13:59

@kaylendog kaylendog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into config? Better integrates with future plans for a configuration module. Otherwise LGTM :3

@codecov

codecov Bot commented Mar 5, 2021

Copy link
Copy Markdown

Codecov Report

Merging #98 (f8f7306) into feat/jcx/major/modules-def (d322b65) will increase coverage by 11.20%.
The diff coverage is 73.33%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           feat/jcx/major/modules-def      #98       +/-   ##
===============================================================
+ Coverage                       23.19%   34.39%   +11.20%     
===============================================================
  Files                              73       63       -10     
  Lines                            3126     2532      -594     
  Branches                          426      286      -140     
===============================================================
+ Hits                              725      871      +146     
+ Misses                           2355     1597      -758     
- Partials                           46       64       +18     
Impacted Files Coverage Δ
...umbdogdiner/stickyapi/bukkit/util/StartupUtil.java 0.00% <0.00%> (ø)
...ogdiner/stickyapi/bungeecord/util/StartupUtil.java 0.00% <0.00%> (ø)
...r/stickyapi/common/translation/LocaleProvider.java 90.62% <ø> (+90.62%) ⬆️
...i/common/configuration/providers/YamlProvider.java 100.00% <100.00%> (ø)
...bdogdiner/stickyapi/common/translation/Locale.java 80.00% <100.00%> (+80.00%) ⬆️
...dumbdogdiner/stickyapi/common/util/StringUtil.java 91.13% <0.00%> (-3.80%) ⬇️
...iner/stickyapi/common/translation/Translation.java 34.73% <0.00%> (+18.94%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d322b65...f8f7306. Read the comment docs.

@jcxldn
jcxldn force-pushed the feat/modules-pr/config branch from 1b3f795 to 8401dd6 Compare March 6, 2021 10:54
@jcxldn
jcxldn force-pushed the feat/modules-pr/config branch from 981453a to 32b638f Compare March 7, 2021 09:46
@jcxldn
jcxldn marked this pull request as ready for review March 7, 2021 17:42

@ZachyFoxx ZachyFoxx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kaylendog kaylendog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor changes :3

import org.junit.jupiter.api.TestInstance.Lifecycle;

@TestInstance(Lifecycle.PER_CLASS)
public class YamlProviderTest {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, a test that forces a concurrent read/write of the config to ensure that behaves as expected.

@jcxldn
jcxldn merged commit 5741c89 into feat/jcx/major/modules-def Mar 17, 2021
@jcxldn
jcxldn deleted the feat/modules-pr/config branch March 21, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking dependencies Pull requests that update a dependency file p0 bug Very high priority bug (server crashing etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants